Skip to content

Fix sline_sline_pos #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

ggnmstr
Copy link
Contributor

@ggnmstr ggnmstr commented Jul 29, 2023

The problem fixed by this pull request is described in Issue #35

@esabol
Copy link
Contributor

esabol commented Jul 29, 2023

Always connect your pull requests to their relevant issue numbers, please. You can do this by mentioning the issue number in the title or description of the PR.

Issue #35

@ggnmstr ggnmstr changed the title fix sline_sline_pos fix sline_sline_pos #35 Jul 29, 2023
@ggnmstr ggnmstr changed the title fix sline_sline_pos #35 fix sline_sline_pos Jul 29, 2023
@esabol
Copy link
Contributor

esabol commented Jul 31, 2023

Seems like a simple enough change, but I'd like to see a test case added which fails without the PR and passes with the PR.

@ggnmstr ggnmstr force-pushed the spherepoly_check_fix branch from b6d7eb0 to a6bd0ff Compare August 1, 2023 05:21
@ggnmstr
Copy link
Contributor Author

ggnmstr commented Aug 1, 2023

Sure, I added a pair of tests to sql/poly.sql
The first one definitely passes with the PR and fails without it (both polygons shouldn't be created).

@esabol
Copy link
Contributor

esabol commented Aug 1, 2023

Looks good to me! Thanks, @ggnmstr!

What do you think, @vitcpp?

@vitcpp
Copy link
Contributor

vitcpp commented Aug 2, 2023

Colleagues, I have some doubts about this change. I think, degenerate polygons can be accepted by most of the algorithms. This PR disables some types of polygons that may affect on user experience - if the old version constructed such degenerate polygons, the new version reports an error. Some user datasets may contain degenerate polygons. Explicit error reporting may stop data pipelines to process some datasets. Give me please some more time to think about it.

@esabol
Copy link
Contributor

esabol commented Aug 2, 2023

This PR disables some types of polygons that may affect on user experience - if the old version constructed such degenerate polygons, the new version reports an error. Some user datasets may contain degenerate polygons. Explicit error reporting may stop data pipelines to process some datasets. Give me please some more time to think about it.

If there's a use-case for supporting degenerate polygons, I'd be open to it, but it seems to me that giving correct results is what should matter. Degenerate polygons are not valid polygons, according to the definition given by the OGC Simple Features Specification. See https://cs.stackexchange.com/a/112703.

@ggnmstr
Copy link
Contributor Author

ggnmstr commented Aug 2, 2023

I found out that PGSphere is kind of inconsistent in terms of degenerate polygons. It doesn't allow to create all types of degenerate polygons:
before-patch
Meanwhile PR I suggest removes some possibilities to create degenerate polygons (connected by 0d coordinate), but not all:
after-patch
So it may be some kind of problem with these 0d coordinates, maybe math, not sure now.
But first, of course, we need to decide whether degenerate polygons should be allowed or not.

@esabol
Copy link
Contributor

esabol commented Aug 2, 2023

Interesting. Thank you for testing that and reporting your findings, @ggnmstr.

@vitcpp
Copy link
Contributor

vitcpp commented Aug 3, 2023

It seems there is a more complex issue with degenerate polygons. Thank you @ggnmstr for your findings. There is the working idea to separate two concepts - storage and algorithms. spoly is a storage for polygon data. The storage may or may not validate the input data because in most cases the storage just used to manipulate data but not to validate it. My position is to allow to keep both valid and invalid data in storage. Data import algorithms just load data into storage objects as is. Some functions to validate and normalize data should be implemented as well. The algorithms assume that the input storage objects contain valid data. Such check is not trivial for polygons. It is the developer responsibility to pass valid data to algorithms. Otherwise, the behaviour is undefined. For most of the algorithms degenerate polygons are ok but it is important to avoid edge crosses that may lead to the wrong results. Overlapping edges may be used to construct polygons with holes (if needed).

Now we have some inconsistent behaviour with spoly. Once we can't create degenerate spoly with 4 points and it is the adopted behaviour I think I'm ok to disable creation of all types of degenerate polygons. Later, if we decide to allow degenerate polygons we can change this behaviour.

Lets wait for PR #22 and then merge it.

@vitcpp
Copy link
Contributor

vitcpp commented Aug 5, 2023

@ggnmstr Could you please rebase the branch to the newer version and start the commit description from the capital letter? We will merge it if tests are passed.

Copy link
Contributor

@vitcpp vitcpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggnmstr Could you, please, start the commit description from the capital letter?

@ggnmstr ggnmstr force-pushed the spherepoly_check_fix branch from a6bd0ff to b6be266 Compare August 5, 2023 17:08
@ggnmstr ggnmstr force-pushed the spherepoly_check_fix branch from b6be266 to 5e7111e Compare August 5, 2023 17:12
@ggnmstr
Copy link
Contributor Author

ggnmstr commented Aug 6, 2023

@vitcpp Done

Copy link
Contributor

@vitcpp vitcpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the tests are passed, I approve it. One trivial request is to start the PR title with a capical letter. @ggnmstr Thank you!

@ggnmstr ggnmstr changed the title fix sline_sline_pos Fix sline_sline_pos Aug 7, 2023
@ggnmstr
Copy link
Contributor Author

ggnmstr commented Aug 7, 2023

Done.
@esabol, @vitcpp, thank you for review.

@vitcpp vitcpp merged commit 7e8a47d into postgrespro:master Aug 8, 2023
@ggnmstr ggnmstr deleted the spherepoly_check_fix branch August 9, 2023 03:19
@ggnmstr ggnmstr restored the spherepoly_check_fix branch August 9, 2023 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants